Skip to content

Add instructions for building the docs #982

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Jul 17, 2020
Merged

Conversation

kandersolar
Copy link
Member

@kandersolar kandersolar commented Jun 13, 2020

  • Closes #xxxx
  • I am familiar with the contributing guidelines
  • Tests added
  • Updates entries to docs/sphinx/source/api.rst for API changes.
  • Adds description and name entries in the appropriate "what's new" file in docs/sphinx/source/whatsnew for all changes. Includes link to the GitHub Issue with :issue:`num` or this Pull Request with :pull:`num`. Includes contributor name and/or GitHub username (link with :ghuser:`user`).
  • New code is fully documented. Includes numpydoc compliant docstrings, examples, and comments where necessary.
  • Pull request is nearly complete and ready for detailed review.
  • Maintainer: Appropriate GitHub Labels and Milestone are assigned to the Pull Request and linked Issue.

@wholmgren wholmgren added this to the 0.8.0 milestone Jul 4, 2020
Copy link
Member

@wholmgren wholmgren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok to merge?

@kandersolar
Copy link
Member Author

Two changes:

  • Mention that both optional and doc need to be installed, previously only said doc
  • pillow was an untracked requirement. I'm not entirely sure why this never popped up before, but I guess all of us already had pillow installed and RTD automatically provides it by default. Anyway it is added to the doc requirements list now.

@wholmgren
Copy link
Member

Mention that both optional and doc need to be installed, previously only said doc

Off the top of my head, I believe this is only true for the 1. the clearsky page's tables requirement and 2. the forecast page's netcdf/siphon requirement. Am I missing anything? If it's just those then maybe add them to the doc requirement. If it's more then maybe make doc an extension of optional instead of instructing users to install both.

pillow was an untracked requirement. I'm not entirely sure why this never popped up before, but I guess all of us already had pillow installed and RTD automatically provides it by default. Anyway it is added to the doc requirements list now.

What makes you think this is the case? I don't have pillow and can build the docs just fine.

@kandersolar
Copy link
Member Author

Good idea about only adding what is needed to the doc requirement. I'll try it out.

What makes you think this is the case? I don't have pillow and can build the docs just fine.

In a fresh environment without pillow the build fails with this:

generating gallery...
generating gallery for auto_examples... [ 14%] plot_greensboro_kimber_soiling.py
Extension error:
Could not import pillow, which is required to rescale images (e.g., for thumbnails): No module named 'PIL'

You can do a build from scratch without pillow or PIL? Sphinx-gallery's readme says it is a (possibly unnecessary) untracked dependency: https://github.com/sphinx-gallery/sphinx-gallery

@wholmgren
Copy link
Member

Indeed building the docs does now require pillow. I was confused about environment/git status. Sorry.

@kandersolar
Copy link
Member Author

Off the top of my head, I believe this is only true for the 1. the clearsky page's tables requirement and 2. the forecast page's netcdf/siphon requirement. Am I missing anything? If it's just those then maybe add them to the doc requirement. If it's more then maybe make doc an extension of optional instead of instructing users to install both.

The example gallery adds another layer since those actually run functions instead of just importing modules. I think the only addition to your list is scipy (for plot_singlediode.py). It's still less than half of the optional list, but that might change as the gallery grows over time. Do you think it's better to keep a lean doc list or just include all of optional so that we don't need to keep track of it?

@wholmgren
Copy link
Member

I think many of us previously agreed (over many issues/prs) that it would be good to add scipy to the base requirements. So that would resolve the concern over the additional dependency. I can go either way on the lean vs. batteries-included doc list.

In either case do we need to change something about the readthedocs configuration so that we can more reliably test the doc build?

@kandersolar
Copy link
Member Author

In either case do we need to change something about the readthedocs configuration so that we can more reliably test the doc build?

I suppose if doc is supposed to set up the complete docs environment, we should tell RTD to install doc, not all: https://github.com/pvlib/pvlib-python/blob/master/readthedocs.yml#L6

That way the RTD build would presumably fail if we add a new requirement but forget to add it to the doc list. With that in place I think I would support keeping doc lean/minimal -- no need to bring in cython and numba just to build the docs.

@kandersolar
Copy link
Member Author

Ok, looks like the RTD build worked with only the updated [doc] list. I also turned off the setting to use preinstalled system packages so that (I think) it only uses the packages specified in setup.py. I left scipy as an optional/doc requirement for now, since I assume making it base requirement will be a bit of a bigger job than just editing setup.py. Anything else to do for this PR?

Copy link
Member

@wholmgren wholmgren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @kanderso-nrel!

@wholmgren wholmgren merged commit 4380445 into pvlib:master Jul 17, 2020
@kandersolar kandersolar deleted the docs_docs branch July 17, 2020 02:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants